Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to generate function body to goto-instrument #1889

Conversation

hannes-steffenhagen-diffblue
Copy link
Contributor

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue commented Feb 27, 2018

This adds the commandline parameters --replace-function-body and --replace-function-body-options. The first gets a regex that defines a set of functions whose body should be replaced; Whereas the latter specifies what kind of body should be generated. The options here are:

  • nondet-return same as havoc without further options
  • assert-false generate a single assert(false); in the body.
  • assume-false generate a single assume(false); in the body (this has a similar effect to --undefined-function-is-assume-false)
  • havoc[,params][,globals:<regex>] which generates a return nondet for non-void functions and assigns nondet to the targets of pointers to non-const if params is set and assignments of nondet to non-const globals matching the regex if it is set.

Other than that, I'm looking for comments on the command line parameters (I've put the function "goals" and the options for the body replacement in separate options for now, but it feels a bit wordy).

@tautschnig
Copy link
Collaborator

May I start with a few questions:

@hannes-steffenhagen-diffblue
Copy link
Contributor Author

hannes-steffenhagen-diffblue commented Feb 28, 2018

@tautschnig

  • Can you elaborate? One ends with the function having no body, the other ends with the function having a body with (possibly) a return nondet; which are "distinguishable" states. Are you saying these should be the same somehow?

  • Block function command line option #1566 Replaces calls to a certain function by assume_false; This behaves similarly to undefined-function-is-assume false but with a certain target function rather than with all undefined functions. This somewhat overlaps with --replace -function-body-options assume-false: The net effect for a "full" goto binary is roughly the same I think, but there are still some differences (for example, you can use replace-function-body on just the goto binary containing the function to replace and then link, whereas you'd need to use make-function-assume-false on all goto-binaries containing the function calls or call it after linking; It also needs to look at all instructions whereas replace-function-body just needs to iterate over the functions, so there might be a slight performance advantage there, though I don't expect it to be particularly large).

  • Havoc non det functions #1585 behaves similarly to --replace-function-body-options havoc,params; The difference being that this PR does the "nondetting" of parameters by actually replacing the function body, whereas in the other PR this is done during abstract interpretation (I'm told actually symex) (and thus needs to be implemented separately for each domain). In that PR @peterschrammel suggested doing this through a goto transformation which this PR is attempting to do. Currently it's only doing the simplest approximation of this, which is just directly assigning nondet to the pointer targets; Some other things that are "thinkable" are recursively assigning nondet values to non-null pointers, struct members and array elements (for constant size arrays), but I wanted to know what others think about this before going too crazy on it.

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue force-pushed the develop-feature_generate_function_bodies branch 3 times, most recently from 43613c1 to 9d874aa Compare February 28, 2018 16:13
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test for function name regex

Add a test for replace-function-body-options skip

Add an option for assert-false-then-assume-false

@@ -1522,8 +1533,13 @@ void goto_instrument_parse_optionst::help()
" --remove-pointers converts pointer arithmetic to base+offset expressions\n" // NOLINT(*)
" --splice-call caller,callee prepends a call to callee in the body of caller\n" // NOLINT(*)
// NOLINTNEXTLINE(whitespace/line_length)
" --undefined-function-is-assume-false\n" // NOLINTNEXTLINE(whitespace/line_length)
" convert each call to an undefined function to assume(false)\n"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed text that should still be there

{
function.body.clear();
generate_parameter_names(function, symbol_table, function_name);
replace_function_body_impl(function, symbol_table, function_name);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "generate_function_body", because the body has already been removed here

goto_functiont &function,
symbol_tablet &symbol_table,
irep_idt function_name) override
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a skip instruction here or rename this to empty

public:
havoc_replace_function_bodiest(
std::vector<irep_idt> globals_to_havoc,
bool havoc_parameters)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use regex for parameter instead of all-in boolean parameter

bool havoc_parameters;
};

std::unique_ptr<replace_function_bodiest> parse_replace_function_body_options(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this to "factory..."

const std::regex &functions_regex,
const std::string &replace_options)
{
messaget messages;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pass this as parameter instead


Module: Replace bodies of goto functions

Author: DiffBlue Limited. All rights reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use "All rights reserved"

@martin-cs
Copy link
Collaborator

For the record, the comments above are a composite of mine and @chrisr-diffblue 's reviews of this done sat with @hannes-steffenhagen-diffblue . Beyond these we are happy to merge this.

@martin-cs
Copy link
Collaborator

There are questions of condensing other options and other functionality. This patch is the first step of a not-as-covert-as-it-was-before-I-started-writing-this attempt to remove the special case code for functions without bodies from the various analyses. In line with this I think we should also:

  1. Redirect --remove-function-body as @tautschnig suggests.
  2. Redirect (and update the description of) --undefined-functions-are-assume-false so that it instead generates function bodies with assume-false in them.

@tautschnig @peterschrammel opinions?

Also @polgreen does the functionality in this PR cover the use cases you had for #1566 and #1585 ?

@martin-cs
Copy link
Collaborator

@hannes-steffenhagen-diffblue : one thing I forgot when we were writing up the review, can we have an option --generate-function-bodies which is --replace-function-body for all functions which don't have a body at the moment.

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue force-pushed the develop-feature_generate_function_bodies branch from 9d874aa to 1c9b407 Compare March 12, 2018 15:32
@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue force-pushed the develop-feature_generate_function_bodies branch from 54ca23e to b80eea6 Compare March 12, 2018 15:58
@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue force-pushed the develop-feature_generate_function_bodies branch 2 times, most recently from 9efca43 to ea1f752 Compare March 12, 2018 16:42
@hannes-steffenhagen-diffblue
Copy link
Contributor Author

@martin-cs I think that's all the comments addressed, except for the option merging thing (maybe separate PR?). Mind taking another look?

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not reviewed tests

^SIGNAL=0$
^VERIFICATION FAILED$
^\[main.assertion.1\] assertion does_not_get_reached: SUCCESS$
^\[should_be_generated.assertion.1\] assertion FALSE: FAILURE$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing new line

std::regex function_regex = std::regex(
only_generate ? cmdline.get_value("generate-function-body")
: cmdline.get_value("replace-function-body"));
status() << "Replacing function bodies" << eom;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this method say "Generating function bodies" if cmdline.get_value("generate-function-body")

@@ -86,7 +86,11 @@ Author: Daniel Kroening, [email protected]
"(undefined-function-is-assume-false)" \
"(remove-function-body):"\
"(splice-call):" \
OPT_REMOVE_CALLS_NO_BODY
OPT_REMOVE_CALLS_NO_BODY \
"(replace-function-body):" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But these options and their documentation in to replace_function_bodies.h to make it easier for it to be reused in different front ends.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a define like some of the other options?

cmdline.isset("replace-function-body") ||
cmdline.isset("generate-function-body"))
{
if(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a mild preference to remove this code from here and put into the constructor of replace_function_bodiest so that the logic of how these parameters are handled is separated from the specific binary being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually the opposite; I had considered doing the parsing of the arguments outside of replace_function_bodiest entirely to avoid passing more "meaningful" strings around than we really have to

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reason was that if someone wishes to re-use this they must copy/paste the interface to provide a consistent experience for the user. Rather than passing raw strings, what if you pass the whole cmdlinet into it? So it is clear they aren't meaningful strings but really the user input.

The down side to my suggestion is becomes a class that is both responsible for generating functions and parsing command line args. Perhaps it would be better to have a third place that takes a cmdline and returns a replace_goto_functionst, but OK with leaving that to the first person who wishes to re-use this code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to support @thk123's position: In order to achieve what #1949 says, we will need this in all front-ends. So we really don't want to reimplement the command-line parsing multiple times. In my opinion, the last suggestion to have "a third place that takes a cmdline and returns..." is the best approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushing the only_generate and function_regex logic into a reusable helper in replace_function_bodies would be preferable.

throw replace_function_body_parse_error("Can't parse \"", options, "\"");
}

void replace_function_bodies(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the interface to this whole functionality, could you add documentation here.

}
};

class assume_false_replace_function_bodiest : public replace_function_bodiest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider documenting each of these classes with:
a) the arugments needed to use them
b) a C style code block of what will be produced

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might also consider writing some really simple unit tests for each of these classes to verify the program produced.

{
if(
parameter.type().id() == ID_pointer &&
!parameter.type().subtype().get_bool(ID_C_constant) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but perhaps this is the same condition as is_constant_or_has_constant_components (e.g. is pointer to a pointer to a constant?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure; Actually it's also possible that this is the better version; i.e. you can absolutely assign a pointer-to-pointer-to-constant. Then again, structs with constant members usually aren't assignable so I'm not 100% clear on what the correct way to do this here is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider writing unit tests to figure out - I think it should be clear what is "correct" when you do this

};

static std::vector<std::string>
tokenize(const std::string &base, char separator)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document? Also surely this code exists somewhere already? Is it not split_string in string_utils.cpp?

}

static void write_to_stream(std::ostream &out)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just to have a convenient method to assemble a string... Didn't end up actually needed it here so I'm removing it, but I figure something along those lines might be useful for string-utils?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? I'll reserve judgement until I see a PR with it being used 😛

const std::vector<std::string> key_value_pair = tokenize(tokens[i], ':');
if(key_value_pair.size() != 2)
{
throw replace_function_body_parse_error(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid introducing throw std::string type statements, consider having a replace_function_body_exceptiont for this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be a child of std::runtime_error? If yes, should the message include __FILE__ and __LINE__?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They both sound like sensible suggestions to me.

@hannes-steffenhagen-diffblue
Copy link
Contributor Author

@thk123 I think I've addressed most of your comments, mind having another look?

#include <util/message.h>

/// Replace function bodies with some default behavior
/// Can generate the following types of function bodies:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain the lhs of this list is the command line argument

/// return nondet_int();
/// }
///
/// nondet-return return nondet for non-void functions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: nondet-return: return nondet for non-void functions (missing colon separating command line from result)

/// @param functions_regex Replace bodies of functions whose names matches regex
/// @param only_generate When true, skip functions that already have a body
/// @param replace_options Options string indicating what body to generate
/// @param messages Destination for status/warning messages
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\param also this documentation should go in the cpp next to the implementation rather than the header (as per coding guidelines)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it would be more useful to split each up to go with the corresponding class but that might just be personal preference

new_param_sym.type = parameter.type();
new_param_sym.base_name = param_base_name;
new_param_sym.mode = ID_C;
auto &function_symbol = symbol_table.lookup_ref(function_name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is clearer if you make the const here explicit.

{
if(
parameter.type().id() == ID_pointer &&
!parameter.type().subtype().get_bool(ID_C_constant) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider writing unit tests to figure out - I think it should be clear what is "correct" when you do this

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue force-pushed the develop-feature_generate_function_bodies branch from 0ae6a9f to 44fb0b1 Compare March 26, 2018 14:29
@hannes-steffenhagen-diffblue
Copy link
Contributor Author

@peterschrammel @thk123 @tautschnig @martin-cs

Can you please check whether or not you feel that your comments have been addressed?

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the updates; still a few smaller issues, but mainly I'm wondering about the set options/the discussion around that, if it has taken place.


Module: Replace bodies of goto functions

Author: DiffBlue Limited.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1919.

};
auto assert_instruction = add_instruction();
assert_instruction->make_assertion(false_exprt());
// assert_instruction->code = code_assertt(false_exprt());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this commented-out code, please remove.

{
throw replace_function_bodies_errort(
"can only use one of --replace-function-body "
"or --generate-function-body");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hannes-steffenhagen-diffblue @peterschrammel - did you discuss this? What was the conclusion, or maybe why was my proposal not acceptable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tautschnig Nobody else said anything about this. It's possible that they missed it. I wouldn't mind implementing it that way, but it wasn't obviously better than the current behaviour from my perspective, I'd prefer at least one other person saying they think it's the better way to do it before going ahead with changing the options at this point.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Poking @martin-cs @chrisr-diffblue with the hope that more people might chime in. Everyone: see the discussion in #1889 (comment) for context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a comment in that discussion thread - but bottom line is that I agree with @tautschnig's proposal.

/// A list of currently accepted command line arguments
/// + the type of bodies generated by them
///
/// assert-false: { __CPROVER_assert(false); }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__CPROVER_assert takes a second argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I should probably just use assert here, the __CPROVER doesn't really add to the legibility of the example


Module: Replace bodies of goto functions

Author: DiffBlue Ltd.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include <util/std_types.h>
#include <util/message.h>
#include <goto-programs/goto_model.h>
#include <util/cmdline.h>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please try to sort internal includes lexicographically.

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New changes seem fine but I think a bit more context on the slightly wrong test would be in order.

havoc_struct(&paramStruct);
assert(globalStruct.non_const == 10);
assert(globalStruct.is_const == 20);
assert(paramStruct.non_const == 11);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not been havoced? (A good technique for trying to verify these tests is to try and write bodies that violate this asserts, for example for this can you not do:

(*WithConstMember).non_const = 20;

const irep_idt &function_name);

private:
/// Generate parameter names for unnamed parameters.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments should go with the implementation

@@ -0,0 +1,410 @@
/*******************************************************************\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this is a move of the original?

{
struct WithConstMember paramStruct = {11, 21};
havoc_struct(&paramStruct);
assert(globalStruct.non_const == 10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact - since you are also havocing globals, shouldn't this assert fail too?

}
else if(!parameter.type().subtype().get_bool(ID_C_constant))
{
warning() << "warning: not havoccing non-const pointer parameter '"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see - this is why the tests are "wrong". I assume there is an issue? Probably worth ensuring this warning appears in the test and there is an appropriate comment on the issue to update the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all intended behaviour for now, I had asked around what people thought the right behaviour should be and it was "just skip structs with const members for now". The other behaviour is also possible, would that be your preference then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the face of it it seems more "correct" as I can easily write a method body for that method that violates that assert without wandering into undefined behaviour or even something particularly odd. That said, I'm not using this tool on a regular basis, it might be in the real world this is simply a more useful behaviour.

@hannes-steffenhagen-diffblue
Copy link
Contributor Author

@thk123 I've now changed the behaviour so that structs, unions and arrays are traversed during havoccing instead of giving up when they contain constant elements.

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue force-pushed the develop-feature_generate_function_bodies branch 4 times, most recently from 177a60a to 38ee140 Compare April 4, 2018 13:35
@hannes-steffenhagen-diffblue
Copy link
Contributor Author

There is some documentation about the feature in doc/cprover-manual.md. Unfortunately this has also lead to merge conflicts, which I will try to resolve now.

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue force-pushed the develop-feature_generate_function_bodies branch 2 times, most recently from e835e6b to 8958d37 Compare April 5, 2018 11:22
@hannes-steffenhagen-diffblue
Copy link
Contributor Author

I have rebased the changes onto the newest develop. Apologies for any inconveniences.

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for changes and docs. I would suggest if the (?!__) thing is so important, you may want to add a warning in the code if the supplied regex matches __CPROVER since most people won't read the manual...

^\[main.assertion.6\] assertion globalUnion.is_const == 10: FAILURE$
^\[main.assertion.7\] assertion paramUnion.non_const == 20: FAILURE$
^\[main.assertion.8\] assertion paramUnion.is_const == 20: FAILURE$
^VERIFICATION FAILED$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing new line

@thk123
Copy link
Contributor

thk123 commented Apr 6, 2018

This is approaching the point where GH refuses to load this page due to too many comments. It might be wise to close this and open a new PR if there are still outstanding comments.

@hannes-steffenhagen-diffblue
Copy link
Contributor Author

@thk123 I might just end up doing that. Technical issues aside, it's starting to get to the point of being difficult to follow along with a lot of the discussion no longer being relevant due to code changes

@tautschnig
Copy link
Collaborator

From my point of view this is very close to be ready to be merged (but others might disagree). I'd actually just suggest to squash the commit series into a few meaningful commits, re-review, approve, and be happy :-)

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue force-pushed the develop-feature_generate_function_bodies branch from 616cfd1 to 29d9882 Compare April 6, 2018 16:43
@hannes-steffenhagen-diffblue
Copy link
Contributor Author

@tautschnig I believe this is in a good state now. Would you mind taking another look at this once CI passes?

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is further room for cleaning up the closely related options, but certainly this is a good piece of work to go in and further work can follow.

One wishlist request: please put a comment in the CHANGELOG file, this is a user-visible feature.

^\[main.assertion.6\] assertion globalUnion.is_const == 10: FAILURE$
^\[main.assertion.7\] assertion paramUnion.non_const == 20: FAILURE$
^\[main.assertion.8\] assertion paramUnion.is_const == 20: FAILURE$
^VERIFICATION FAILED$
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Github says there's no newline here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -1522,9 +1535,10 @@ void goto_instrument_parse_optionst::help()
" --check-invariant function instruments invariant checking function\n"
" --remove-pointers converts pointer arithmetic to base+offset expressions\n" // NOLINT(*)
" --splice-call caller,callee prepends a call to callee in the body of caller\n" // NOLINT(*)
" --undefined-function-is-assume-false\n"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we mark this as deprecated (possibly be removing it from help output, but leaving it in the .h file)? Also: is it now implemented via the new options? (Just being lazy, I should be able to figure out once I've completely read the code.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The effect can be replicated with --generate-function-body '.*' --generate-function-body-options assume-false

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed - but then we could get rid of the code implementing undefined_function_abort_path, couldn't we?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hannes-steffenhagen-diffblue I'd leave it to you whether to include such changes in this PR or instead just create an issue and make someone else (such as myself...) fix it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created #2070 to cover the additional cleanup and will merge this PR now.

@@ -86,7 +88,9 @@ Author: Daniel Kroening, [email protected]
"(undefined-function-is-assume-false)" \
"(remove-function-body):"\
"(splice-call):" \
OPT_REMOVE_CALLS_NO_BODY
OPT_REMOVE_CALLS_NO_BODY \
OPT_REPLACE_FUNCTION_BODY
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might not be worth renaming to OPT_GENERATE...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue force-pushed the develop-feature_generate_function_bodies branch from 29d9882 to ebfaf5a Compare April 9, 2018 10:56
@hannes-steffenhagen-diffblue
Copy link
Contributor Author

@tautschnig I added a short reference to the new option in CHANGELOG under a new "5.9" section

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue force-pushed the develop-feature_generate_function_bodies branch from ebfaf5a to 7952f2c Compare April 9, 2018 11:40
Copy link
Contributor

@chrisr-diffblue chrisr-diffblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks for all the extra work and polishing on this PR.

@polgreen
Copy link
Contributor

I know this PR has already been merged, but I have a question about the documentation:

" /// Replace the function body with one based on the implementation
/// This will work the same whether or not the function already has a body"

Does "implementation" have a specific meaning, I thought it just meant the actual function body? I'm not sure how you would replace the function body with one based on the function body if there is no function body, or, if there is a body, how the body that it replaces the body with is based on the original body?

smowton added a commit to smowton/cbmc that referenced this pull request May 9, 2018
ad62682 Merge pull request diffblue#2071 from thk123/refactor/split-string-unit-tests
fc8ba88 Revert to aborting precondition for function inputs
3e2ab6f Merge pull request diffblue#2080 from diffblue/java-bytecode-dependency
6ff1eec cbmc: remove dependency on java_bytecode
0bff19b Merge pull request diffblue#2049 from karkhaz/kk-factor-goto-model-processing
79e3b25 Merge pull request diffblue#2084 from tautschnig/has_subtype-test
cd45b0b Test has_subtype on recursive data types
85ac315 Merge pull request diffblue#2082 from thomasspriggs/default_dstring_hash
28c2e8b Merge pull request diffblue#2065 from tautschnig/be-constructor
afa6023 Merge pull request diffblue#2061 from tautschnig/simplify-extractbits
014d151 Factor out getting & processing goto-model
06b3adc Merge pull request diffblue#2077 from peterschrammel/stable-tmp-var-names
0b3170d Stabilize clinit wrapper function type parameters
3cd8bf4 Temporary vars tests for goto-diff
9f0626c  Prefix temporary var names with function id
ca678aa More permissive regression tests regarding tmp var suffixes
47951ca Merge pull request diffblue#2079 from romainbrenguier/bugfix/has-subtype-recursion
dd73b1a Specify default hash function of `dstringt` to STL.
fe8e589 Avoid infinite recursion in has_subtype
00b9bf6 Merge pull request diffblue#2051 from svorenova/generics_tg2717
cd4bfc3 Merge pull request diffblue#2078 from romainbrenguier/bool-literal-in-while-loop
67ea889 Use bool literal in while loop
d229ad9 Merge pull request diffblue#2056 from diffblue/fix-regression-cbmc-memcpy1
506faf0 Refactor a function for base existence
617d388 Utility functions for generic types
c07e6ca Update generic specialization map when replacing pointers
ed26d0a Merge pull request diffblue#2058 from peterschrammel/stable-disjuncts
b663734 Simplify extractbits(concatenation(...))
b091560 Typing and refactoring of simplify_extractbits
49ad1bd Merge pull request diffblue#974 from tautschnig/fix-assert-encoding
16e9599 Merge pull request diffblue#2063 from tautschnig/has-subtype
950f58b Merge pull request diffblue#2060 from tautschnig/opt-local-map
4222a94 Regression tests for unstable instanceof and virtual method disjuncts
b44589e Make disjuncts in instanceof removal independent of class loading
3afff86 Make disjuncts in virtual method removal independent of class loading
a385d9b Allowed split_string to be used on whitespace if not also trying to strip
fe4a642 Merge pull request diffblue#2062 from tautschnig/no-has-deref
145f474 Adding tests for empty string edge cases
07009d4 Refactored test to run all combinations
252c24c Migrate old string utils unit tests
e87edbf Removing wrong elements from the make file
b165c52 make test work on 32-bit Linux
b804570 Merge pull request diffblue#2048 from JohnDumbell/improvement/adding_null_object_id
61f14d8 Merge pull request diffblue#1962 from owen-jones-diffblue/owen-jones-diffblue/simplify-replace-java-nondet
fdee7e8 Merge pull request diffblue#2059 from tautschnig/generalise-test
4625cc5 Extend global has_subexpr to take a predicate as has_subtype does
e9ebd59 has_subtype(type, pred, ns) to search for contained type matching pred
1f1f67f Merge pull request diffblue#1889 from hannes-steffenhagen-diffblue/develop-feature_generate_function_bodies
048c188 Add unit test for java_replace_nondet
0fe48c9 Make remove_java_nondet work before remove_returns
bcc4dc4 Use byte_extract_exprt constructor
a1814a3 Get rid of thin (and duplicate) has_dereference wrapper
4122a28 Test to demonstrate assert bug on alpine
d44bfd3 Also simplify assert structure found on alpine linux
c5cde18 Do not generate redundant if statements in assert expansions
4fb0603 Make is_skip publicly available and use constant argument
5832ffd Negative numbers should also pass the test
3c23b28 Consistently disable simplify_exprt::local_replace_map
da63652 Merge pull request diffblue#2054 from romainbrenguier/bugfix/clear-equations
d77f6a2 Merge pull request diffblue#1831 from NathanJPhillips/feature/class-annotations
60c8296 Clear string_refinement equations (not dependencies)
314ed53 Correcting the value of ID_null_object
751a882 Factor out default CBMC options to static method
6f24009 Can now test for an option being set in optionst
9a8d937 Add to_annotated_type and enable type_checked_cast for annotated_typet
ca77b4e Add test for added annotations
b06a27d Introduce abstract qualifierst base class
e6fb3bf Pretty printing of java_class_typet
e22b95b Fix spurious virtual function related keywords
3ac6d17 Add type_dynamic_cast and friends for java_class_typet
ce1f4d2 Add annotations to java_class_typet, its methods and fields
f84753d Merge pull request diffblue#2042 from hannes-steffenhagen-diffblue/add_deprecate_macro
7a38669 Merge pull request diffblue#2017 from NathanJPhillips/feature/overlay-classes
75a4aec Revert "the deprecation will need to wait until codebase is clean"
67735b5 Disable deprecation warnings by default
0764f77 Merge pull request diffblue#2036 from romainbrenguier/id_array_list
690b606 Merge pull request diffblue#2039 from peterschrammel/fix-duplicate-msg-json-ui
bba17d9 the deprecation will need to wait until codebase is clean
822c757 Regression test for redundant JSON message output
de0644d Only force end of previous message if there actually is one.
5a637bf Merge pull request diffblue#2037 from hannes-steffenhagen-diffblue/add_deprecate_macro
bff456a Merge pull request diffblue#2040 from tautschnig/remove-swp
87ebe90 Remove vim temp file
228c019 Fix duplicate message output in json-ui
0a2c43e Add DEPRECATED to functions documented as \deprecated
47f4b35 interval_sparse_arrayt constructor from array-list
026c4ca Declare an array_list_exprt class
50a2696 Define ID_array_list
513b67a Merge pull request diffblue#2038 from romainbrenguier/bugfix/assign-at-malloc-site
c207106 Test with array of strings
60183a3 Assign string at malloc site
116fffd Add DEPRECATED macro to mark deprecated functions and variables
7952f2c Add option to generate function body to goto-instrument
3d4183a Add ability to overlay classes with new definitions of existing methods
dbc2f71 Improve code and error message in infer_opaque_type_fields
7c0ea4d Tidied up java_class_loader_limitt

git-subtree-dir: cbmc
git-subtree-split: ad62682
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants